Skip to content

add tusk solution#1520

Open
prokopenkooleh wants to merge 2 commits into
mate-academy:masterfrom
prokopenkooleh:develop
Open

add tusk solution#1520
prokopenkooleh wants to merge 2 commits into
mate-academy:masterfrom
prokopenkooleh:develop

Conversation

@prokopenkooleh

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core requirement is still violated: onPageChange is being called even when the user clicks the already active page, which conflicts with checklist item #5 (the callback must fire only when the page actually changes). This is a functional behavior issue, not just style, and can cause unnecessary state updates and test failures.

Critical issues to fix:

  • In src/components/paganation/Pagination.tsx, inside the handler for numbered page links (around the onClick={() => onPageChange(page)}), add a guard so that the callback is only invoked when page !== currentPage. For example: onClick={() => { if (page !== currentPage) onPageChange(page); }}.
  • In src/App.tsx, if you have a wrapper like handlePageChange(page) that forwards directly to setCurrentPage(page) or onPageChange(page), make sure it also respects this rule and doesn’t trigger redundant updates when the page is already selected.

Everything else in your implementation (stateless Pagination, prev/next disabling with li.disabled and aria-disabled, URL sync, per-page selector, item slicing, and info text formatting) looks solid and aligns well with the requirements. Once you add the equality check and ensure onPageChange never fires for the current page, your solution should fully meet the task standards. Keep up the good work—this is a small but important tweak to reach full compliance.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/App.tsx

<p className="lead" data-cy="info">
Page 1 (items 1 - 5 of 42)
Page {currentPage} (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handler always calls onPageChange(page), even when page is equal to currentPage. Checklist item #5 requires that onPageChange be triggered only when the page actually changes. Consider short‑circuiting the handler when page === currentPage so that no callback is fired in that case.

href={`#${page}`}
onClick={e => {
e.preventDefault();
onPageChange(page);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to checklist item #5, onPageChange must be called only when the page actually changes. Here you always call onPageChange(page) even if page === currentPage, which can cause redundant updates. Add a guard (e.g. compare page with currentPage) before invoking the callback.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because the Cypress end‑to‑end tests are currently all failing due to missing data-cy attributes, which is a critical blocker for the task completion. Functionally, your pagination and router integration look solid: Pagination is stateless, onPageChange is properly guarded, URL sync via useSearchParams works, and the per‑page selector and info text behave correctly. However, the tests expect specific attributes on elements, and without them the test suite cannot verify your otherwise correct behavior.

Critical issues to fix:

  • Add data-cy="pageLink" to each rendered page number link in src/components/paganation/Pagination.tsx (the <a> elements inside your page <li>s).
  • Add data-cy="prevLink" to the previous page control and data-cy="nextLink" to the next page control.
  • Ensure the per-page <select> in App.tsx has data-cy="perPageSelector" exactly (it sounds like you already do this, but please double‑check spelling and casing).

Once these attributes are in place and the tests can find the elements, your implementation should pass the E2E suite and fully meet the requirements. The core logic looks very good, so this is mainly about wiring the DOM to the test harness correctly—nice work so far, and this should be a quick fix for you.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@Anton-Kuchmasov Anton-Kuchmasov left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests are passed locally:

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants